Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add button check to validateUILayout #11

Closed
wants to merge 3 commits into from

Conversation

denisonbarbosa
Copy link
Member

According to the specification, the broker can only specify a button for the layout if wait == true, otherwise the layout is invalid and an error should be returned.

@denisonbarbosa denisonbarbosa requested a review from a team as a code owner August 8, 2023 12:38
@denisonbarbosa denisonbarbosa removed the request for review from a team August 8, 2023 12:41
@denisonbarbosa denisonbarbosa marked this pull request as draft August 8, 2023 12:41
According to the specification, the broker can only specify a button for
the layout if wait == true, otherwise the layout is invalid and an error
should be returned.
Now that wait == true is required in order to also use a button, the
layout should specify it.
@denisonbarbosa denisonbarbosa marked this pull request as ready for review August 8, 2023 13:04
@denisonbarbosa denisonbarbosa requested a review from a team August 8, 2023 13:04
@@ -190,6 +190,9 @@ func validateUILayout(layout map[string]string) (r map[string]string, err error)
if !slices.Contains([]string{"true", "false", ""}, wait) {
return nil, fmt.Errorf("'wait' does not match allowed values for this type: %v", wait)
}
if button != "" && wait != "true" {
return nil, errors.New("button is not allowed if wait is not true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, do you mind aligning to fmt.Errorf since it's used throughout the function? Or changing the other calls with no interpolations to use errors.New as well (in a separate commit obviously).

We were using fmt.Errorf() to create errors without any formatting, so
it's better to switch to errors.New().
@didrocks
Copy link
Member

didrocks commented Aug 9, 2023

As said during the meeting, the spec is now amended, and this button will do another select check. This is then not useful anymore and we should not call IsAuthorized to signal sms changes, but rather when we enter the selection mode. Discaring.

@didrocks didrocks closed this Aug 9, 2023
@didrocks didrocks deleted the form-button-validation branch September 14, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants